Skip to content

Conversation

@mikemckiernan
Copy link

Initiated by dmellado.

This is a followup PR to Daniel's: #28549.
Lingo and restructure changes are underway.

/hold

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 15, 2021
@jboxman jboxman added this to the Future Release milestone Jan 15, 2021
@jboxman
Copy link
Contributor

jboxman commented Jan 15, 2021

@mikemckiernan, maybe this is useful too: openshift/egress-router-cni#25

@openshift-docs-preview-bot

The preview will be available shortly at:

Copy link
Contributor

@jboxman jboxman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some notes for your review.

== Egress router modes

In _redirect mode_, an egress router pod sets up iptables rules to redirect traffic from its own IP address to one or more destination IP addresses. Client pods that need to use the reserved source IP address must be modified to connect to the egress router rather than connecting directly to the destination IP.
In _redirect mode_, an egress router pod configures `iptables` rules to redirect traffic from its own IP address to one or more destination IP addresses. Client pods that need to use the reserved source IP address must be modified to connect to the egress router rather than connecting directly to the destination IP.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd have to have a style check on iptables; I'd almost be tempted to say Netfilter instead, but iptables is the more common way of referring to packet filtering on Linux.

+
After you create the service, your pods can connect to the service. Their connections are redirected to the corresponding ports on the destination IP address and the connections originate from the reserved source IP address.

.Verification steps
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome.


.Verification steps

To verify that the egress router pod started and Multus attached a second network interface, complete the following procedure:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe;

s/second/secondary/?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If secondary is consistent with elsewhere, then certainly. One thought this triggers is that I don't think I indicated that a new default route is added and the route uses the primary (?) interface, eth0.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the route thing is a different topic, please see: openshift/egress-router-cni#27
but you can check that the routes will be as follows

default via 192.168.111.1 dev net1 
10.128.2.0/23 dev eth0 proto kernel scope link src 10.128.2.164 
192.168.111.0/24 dev net1 proto kernel scope link src 192.168.111.200 
192.168.111.1 dev net1

being 192.168.11.1 the gateway and net1the macvlan interface

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a noob, the output of ip ro show was clearest to me. The only way that I know to show that output is oc exec -it... but A) doesn't that add some tainted cooties to the pod? and B) I'm unsure if that teeters from verification into troubleshooting. I'll noodle with Jason or other writers if I find that my understanding of "A" is incorrect.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you could do this time to get to the ip ro showoutput is just oc rsh the actual pod ip, and then run the command. You can also go to the network namespace, enter it and run the command from the host as well. If you feel that that would help verification I'm happy to provide detailed commands (which IIRC are in the developer's guide as well). TL; DR the idea about the routing is, everything but the cluster network, out using the provided gateway, and that gateway out using the macvlan interface.

ifeval::["{context}" == "deploying-egress-router-cni-redirection"]
:redirect:
:router-type: redirect
:image-name: FIXME
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might vary between OCP and OKD, as well.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danielmellado , @weliang1 , I'm too confused to even hazard a guess. Will Multus in OKD also copy the binary? If yes, do we need a "wait/sleep" image and are there separate images for OCP and OKD.

I'm really looking forward to the day when I can answer some of my own questions.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm, well, the image really gets copied by the multus' daemonset on CNO. Not sure if that really applies to OKD

----
<.> The name of the network attachment definition is used later in the specification for the egress router pod.

<.> The `addresses` key specifies the reserved source IP address to use with the additional network interface.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be worth checking if this array can hold only a single value, or if multiple values are permissible.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

multiple values will be permissible, stay with 1 for 4.7 just in case

@mikemckiernan
Copy link
Author

@danielmellado , @weliang1 , can you review the changes too? Some questions off the bat:

  • dev doc in egress-router-cni/Online ==>> Master #25 indicates the binary is delivered with the Multus pod image. The quay.io image that is suggested on line 114 results in a "Failed to pull image" message. What is the image that OKD would use? Multus?
  • Is there a "wait/sleep" image for the ER pod that a customer needs to use?
  • Is only CIDR accepted for addresses and destinations? I receive an error with out the ipnet portion.
  • Addresses and destinations sounds plural. Does ER CNI support > 1 for each and does it make sense? I see taking the first one of each slice in pkg/macvlan/macvlan.go in macvlanCmdGo, but I'm using a lot of imagination.
  • Using a svc is important, right? Is the example that uses 80 & 443 silly? Would 3306 for MySQL be more realistic? I'll take anything from a test.

I'll think of another question right after I add this comment. Thanks.

Copy link

@danielmellado danielmellado left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, left a few comments/answers around! Feel free pinging if you need any additional ones!

----
<.> The name of the network attachment definition is used later in the specification for the egress router pod.

<.> The `addresses` key specifies the reserved source IP address to use with the additional network interface.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

multiple values will be permissible, stay with 1 for 4.7 just in case

ifeval::["{context}" == "deploying-egress-router-cni-redirection"]
:redirect:
:router-type: redirect
:image-name: FIXME

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm, well, the image really gets copied by the multus' daemonset on CNO. Not sure if that really applies to OKD


.Verification steps

To verify that the egress router pod started and Multus attached a second network interface, complete the following procedure:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the route thing is a different topic, please see: openshift/egress-router-cni#27
but you can check that the routes will be as follows

default via 192.168.111.1 dev net1 
10.128.2.0/23 dev eth0 proto kernel scope link src 10.128.2.164 
192.168.111.0/24 dev net1 proto kernel scope link src 192.168.111.200 
192.168.111.1 dev net1

being 192.168.11.1 the gateway and net1the macvlan interface

@danielmellado
Copy link

@danielmellado , @weliang1 , can you review the changes too? Some questions off the bat:

* [X]  dev doc in egress-router-cni/#25 indicates the binary is delivered with the Multus pod image.  The quay.io image that is suggested on line 114 results in a "Failed to pull image" message.  What is the image that OKD would use? Multus?

I've just tried to pull that image and it works, maybe an issue on the docker rate limiting?

podman pull quay.io/openshift/origin-egress-router-cni:latest

* [X]  Is there a "wait/sleep" image for the ER pod that a customer needs to use?

As long as the pod gets the label to get attached to the NAD that is previously created, any image can be used. For the record, we can mention that such pod would need to be running.

* [X]  Is only CIDR accepted for addresses and destinations?  I receive an error with out the ipnet portion.

Yeah, we expect CIDR notation for both addresses and destinations, as per the gateway, we assume a /32 for it. See yet another example of a NAD configuring the CNI plugin

---
apiVersion: "k8s.cni.cncf.io/v1"
kind: NetworkAttachmentDefinition
metadata:
  name: ovn-egressrouter-redirect
spec:
  config: '{
    "cniVersion": "0.4.0",
    "type": "egress-router",
    "name": "ovn-egressrouter-redirect",
    "ip": {
      "addresses": [
        "192.168.111.200/24"
        ],
      "destinations": [
        "172.217.15.78/32"
      ],
      "gateway": "192.168.111.1"
      }
    }'
* [X]  Addresses and destinations sounds plural.  Does ER CNI support > 1 for each and does it make sense?  I see taking the first one of each slice in pkg/macvlan/macvlan.go in macvlanCmdGo, but I'm using a lot of imagination.

We'll be adding that part soon, as of now yeah, it just takes one, (https://github.com/openshift/egress-router-cni/blob/master/pkg/macvlan/macvlan.go#L297)

* [X]  Using a svc is important, right?  Is the example that uses 80 & 443 silly?  Would 3306 for MySQL be more realistic?  I'll take anything from a test.

I just wanted to be somehow coherent with the blogpost explaining the legacy ER [1], so I chose to add that, we could change that to MySQL port or whatever ;)

I'll think of another question right after I add this comment. Thanks.
[1] https://www.openshift.com/blog/accessing-external-services-using-egress-router

@netlify
Copy link

netlify bot commented Jan 29, 2021

Deploy preview for osdocs ready!

Built with commit 864fd3d252b968e23c4e314dd627c508999b97cb

https://deploy-preview-28609--osdocs.netlify.app

@mikemckiernan
Copy link
Author

The HTML is available at the foll URL and by following the link at the bottom of that page: https://deploy-preview-28609--osdocs.netlify.app/openshift-enterprise/latest/networking/ovn_kubernetes_network_provider/using-an-egress-router-ovn.html

Next:

  • While I understand that any image can be used, I have no idea what image a real customer deployment would be willing to fetch and start. Any suggestions for who we can contact?
  • Please Ctrl+F for FIXME. Some of those are dups because they are related to image names.

Thanks again.

@netlify
Copy link

netlify bot commented Jan 29, 2021

Deploy preview for osdocs ready!

Built with commit ee39e82

https://deploy-preview-28609--osdocs.netlify.app

@mikemckiernan
Copy link
Author

@weliang1, PTAL. @danielmellado, I welcome another look, but want to thank you for all your help so far.

At this point, any mistakes are invisible to me.

@danielmellado
Copy link

/assign @weliang1

Copy link

@weliang1 weliang1 Feb 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danielmellado Do we also need check iptables rules here by running below cmd?

[root@ip-10-0-134-247 /]# iptables-save -t nat
*nat
:PREROUTING ACCEPT [0:0]
:INPUT ACCEPT [0:0]
:POSTROUTING ACCEPT [0:0]
:OUTPUT ACCEPT [0:0]
-A PREROUTING -i eth0 -j DNAT --to-destination 172.217.7.142
-A POSTROUTING -o net1 -j SNAT --to-source 10.0.134.248
COMMIT

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's basically up to the user, both commands would be equivalent ;)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/LGTM

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 3, 2021
@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 5, 2021
@mikemckiernan
Copy link
Author

@openshift/team-documentation, PTAL.

Jason already added the right labels and milestone.

/hold cancel
/unassign @weliang1

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 5, 2021
Copy link
Contributor

@bobfuru bobfuru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few minor comments but overall this is great work!

* Initiated by dmellado.
* Add topic for deploying a redirect egress router.
* Reorganized for modularization.
* Feedback from Jason and Daniel.
* Update the image names.  The pod just needs to remain running.
* Eng and QE reviewed. Some passive-to-active voice updates too.
* Peer review from Bob.
@bobfuru
Copy link
Contributor

bobfuru commented Feb 5, 2021

LGTM

@bobfuru bobfuru merged commit cc2a1d5 into openshift:master Feb 5, 2021
@bobfuru
Copy link
Contributor

bobfuru commented Feb 5, 2021

/cherrypick enterprise-4.7

@openshift-cherrypick-robot
Copy link

openshift-cherrypick-robot commented Feb 5, 2021

@bobfuru: new pull request created: #29200

Details

In response to this:

/cherrypick enterprise-4.7

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

branch/enterprise-4.7 size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants